-
-
Notifications
You must be signed in to change notification settings - Fork 758
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Less strict verification #795
Less strict verification #795
Conversation
@@ -36,14 +32,14 @@ public static PointOption point(int xOffset, int yOffset) { | |||
* @return self-reference | |||
*/ | |||
public T withCoordinates(int xOffset, int yOffset) { | |||
checkArgument(xOffset >= 0, format(ERROR_MESSAGE_TEMPLATE, "X")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it temporary change for backward compatibility with older servers versions?
I think it is not necessary to remove it. Just comment it, maybe with the TODO mark.
And before the publishing of the 6.0.0 we can remove //
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a situation when it is necessary to provide negative absolute coordinates. For example, if the device has two screens and the second screen is located on the left side of the main one. That is why I'd consider this restriction as not very useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it is possible that one wants to drag an element to an off-screen area and negative coordinates might also have sense for such case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @mykola-mokhnach. I tried using java-client 6.0.0-BETA1, but I was needed to rollback due to the new limitation: we have cases when actions start at the screen, but end out off screen.
@@ -44,8 +44,6 @@ public void invalidOptionsArgumentsShouldFailOnAltering() throws Exception { | |||
final List<Runnable> invalidOptions = new ArrayList<>(); | |||
invalidOptions.add(() -> waitOptions(ofMillis(-1))); | |||
invalidOptions.add(() -> new ElementOption().withCoordinates(0, 0).withElement(null)); | |||
invalidOptions.add(() -> new PointOption().withCoordinates(0, -1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just comment I think
Ok |
Will publish it on this weekend |
Change list
moveTo
fix is not deployed to Appium yet, so it's OK to allow clients to still use relative coordinates. I've also added a note about the problem into the docstring.Types of changes